[RTG] Add ElaborationPass #7876
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maerhart
force-pushed
the
maerhart-rtg-elaboration-pass
branch
from
36dfd4c to
2633cab
Compare
youngar
reviewed
Nov 26, 2024
Comment on lines
503
to
587
| // If the test requries nothing from a target, we can always run it. | ||
| if (test.getTarget().getEntryTypes().empty()) | ||
| continue; |
There was a problem hiding this comment.
So if a test does not have any requirements we won't run it on each target, just once in general?
There was a problem hiding this comment.
There are two things to consider:
- The unelaborated test is the same for all targets, so do we want to randomize it once for each target or randomize it once and use that for each target? If the latter, then we don't have to copy it. But I guess the earlier approach would be the more consistent one, right?
- Currently I assume that all tests for all targets will be run on the same system because there is no way to distinguish them after this pass. If we want to have targets for different systems in the same MLIR file, we'd need some scoping, add the target that the test specialized for as an attribute, or some other way to distinguish them. As long as we don't do that, we don't need to duplicate a test that has the same content.
I guess I will change it to be copied and independently randomized for each target, but leave point 2 for a future PR.
There was a problem hiding this comment.
I don't really know whats best, what you had originally is probably good! We can revisit these points in the future easily enough.
maerhart
force-pushed
the
maerhart-rtg-test-and-target
branch
3 times, most recently
from
506937f to
14a434c
Compare
maerhart
force-pushed
the
maerhart-rtg-elaboration-pass
branch
from
2633cab to
b89ad7c
Compare
maerhart
force-pushed
the
maerhart-rtg-elaboration-pass
branch
from
b89ad7c to
df7c685
Compare
rwy7
reviewed
Dec 2, 2024
maerhart
force-pushed
the
maerhart-rtg-elaboration-pass
branch
from
df7c685 to
f8cb26a
Compare
youngar
reviewed
Dec 2, 2024
maerhart
force-pushed
the
maerhart-rtg-elaboration-pass
branch
3 times, most recently
from
07d1ea4 to
07d7892
Compare
maerhart
commented
Dec 3, 2024
There was a problem hiding this comment.
- I have removed handling of sequences to simplify this PR and will create another PR with those features. Support for sequences is far from trivial and having them in another PR allows us to focus on sets in this PR.
- I've removed opaque/unevaluated values for now and will also add them in a future PR such that we can focus on the issues that this can introduce in a dedicated PR
- I've added an
AttributeValueto be able to add tests now that we don't support sequences anymore (this is easier to reason about and we need it anyway) - I've fixed the dominance issues by introducing a materializer that properly caches materialized values per block and by removing opaque value support for now
- I've fixed an issue where I iterated over a non-deterministic set order. Basically we need to keep track of some deterministic order for sets in general not just in debug mode. Thus I replaced it with a SetVector.
- Removed the
rtg.elaborationattribute support in favour ofrtg.elaboration_custom_seedwith specifies a seed just for that particular op and removed debug mode. Being able to specify the index in the set is more complicated to handle for not a lot of benefit. The purpose for the attribute is to make it easier to write tests (adding a test at some point should not influence the random selection at another place). - I've added some additional debug printing support for which I had a separate PR, but I think it's valuable to have it from the start and it also included some bug fixes.
Comment on lines
+296
to
+353
| template <typename ValueTy, typename... Args> | ||
| void internalizeResult(Value val, Args &&...args) { | ||
| // TODO: this isn't the most efficient way to internalize | ||
| auto ptr = std::make_unique<ValueTy>(std::forward<Args>(args)...); | ||
| auto *e = ptr.get(); | ||
| auto [iter, _] = interned.insert({e, std::move(ptr)}); | ||
| state[val] = iter->second.get(); | ||
| } |
There was a problem hiding this comment.
I haven't updated the internalization logic to a more efficient approach yet. My strategy would be to wait until we have a big input example that we can use the profile this part to find the most efficient implementation. But let me know if you would like to see an improvement already in this PR.
| // inserting an object of a derived class of ElaboratorValue. | ||
| // The custom MapInfo makes sure that we do a value comparison instead of | ||
| // comparing the pointers. | ||
| DenseMap<ElaboratorValue *, std::unique_ptr<ElaboratorValue>, InternMapInfo> |
There was a problem hiding this comment.
As you pointed out yesterday, this is not particularly elegant. The reason I did it that way is because unique pointers don't seem to work as set or map keys because the copy constructor is deleted. Maybe I'm just missing something to make this work?
Alternatively, we could just use raw pointers that we deallocate manually in the destructor, but that's not very elegant either.
Any concrete suggestions on how to improve this?
maerhart
commented
Dec 3, 2024
maerhart
commented
Dec 3, 2024
maerhart
force-pushed
the
maerhart-rtg-elaboration-pass
branch
2 times, most recently
from
9f88cb6 to
f9c513a
Compare
fabianschuiki
approved these changes
Dec 4, 2024
maerhart
force-pushed
the
maerhart-rtg-elaboration-pass
branch
from
f9c513a to
505d2a0
Compare
maerhart
commented
Dec 6, 2024
maerhart
force-pushed
the
maerhart-rtg-elaboration-pass
branch
from
505d2a0 to
999992a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add a pass that lowers the random parts of RTG IR.